Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StreamReader/Writer are disposing Request/Response body #7

Merged

Conversation

MaxDeg
Copy link
Contributor

@MaxDeg MaxDeg commented Nov 7, 2019

The usage of use on the StreamReader and StreamWriter is disposing the Body. This is causing issue if middleware are trying to use it afterward.
I think it's best to not dispose Stream that we do not own.
This fix the issue. I just added the leaveOpen parameter to the reader/writer to not close inner stream on dispose

@@ -53,7 +53,7 @@ type ThothSerializer (?isCamelCase : bool, ?extra : ExtraCoders, ?skipNullField

static member ReadBodyRaw (ctx: HttpContext) =
task {
use stream = new System.IO.StreamReader(ctx.Request.Body, Utf8EncodingWithoutBom)
use stream = new System.IO.StreamReader(ctx.Request.Body, Utf8EncodingWithoutBom, true, DefaultBufferSize, true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @MaxDeg ,

I don't know the SteamReaderAPI, why is this one having 3 new parameters instead of just one like the others?
Should we use the 3 parameters or use something like ?leaveOpen = true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

Sadly StreamReader only have one constructor that allow to provide leaveOpen option as last parameter. None of those parameters are optional so we need to provide them all.

@MangelMaxime MangelMaxime merged commit 071d34e into thoth-org:master Nov 12, 2019
@MangelMaxime
Copy link
Contributor

Thank you,

Released as version 3.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants